You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
fix: tie the feed cache to its feed and expire it, clamp cache_interval - #52
Fixes the cache problems from the independent review of #46, plus the README items from that review.
Cache
The app cached the feed in one shared localStorage key (rssStore). It never recorded which feed the cache came from, and never checked its age.
Situation
Before
Now
rss_url changed and the new feed fails
The old feed's stories under the new title
Error screen
Feed down for days
Cached stories stay up indefinitely
Cached stories for up to 24 hours, then the error screen
Feed returns no items
Error screen, even with a good cache
The cached stories
cache_interval: 0 (or 1, or a huge value)
Refetches in a tight loop (huge values overflow setInterval, which then fires immediately)
Clamped to 60–86400 s, with a console warning
Caches are now stored per feed URL (rssStore:<url>), each with its URL and save time. When the app saves a cache, it removes the legacy key and other feeds' expired caches, so storage doesn't grow when rss_url changes.
One trade-off: the legacy cache doesn't say which feed it belongs to, so it's never used. If a screen's feed is down on its first load after this ships, it shows the error screen instead of the old cache, until the first successful fetch.
A correction to the review: a non-numeric cache_interval was not a loop, because getSettingWithDefault already falls back to the default for those. The loop came from 0, small values and Infinity.
README
Releasing: any v<digit> tag, including pre-release tags like v26.11.0-rc.1, deploys to production. This comes from the shared edge-apps-actions tag filter, which every app uses.
Overrides: use an app key with them. With the key empty, an override changes every feed app.
Redeploying the main app: re-run the latest tag's workflow run. A manual run from a tag does nothing for it, which the README previously implied it did.
cache_interval: the 60–86400 range and the 24-hour cache are documented in the README table and in the setting's help text in screenly.yml.
Testing
Unit tests: 27 pass, including new ones for per-feed caches, expiry, legacy-key cleanup, pruning, malformed caches and the interval clamp.
In a real browser (Playwright, real localStorage), each scenario in the table behaves as listed, plus the legacy-only case (error screen). With cache_interval: 0, the app fetched once in 3 s.
- The cache was one shared localStorage key with no feed URL or age
check. After rss_url changed and the first fetch failed, the old feed's
stories showed under the new title, and during an outage cached stories
stayed up indefinitely. Caches are now kept per feed URL, ignored after
24 hours, and the legacy key and other feeds' expired caches are removed
on save.
- An empty feed falls back to the cache like a failed fetch, instead of
blanking the screen.
- cache_interval is clamped to 60-86400 seconds. 0 or a tiny value
refetched the feed in a tight loop, and a huge one overflowed
setInterval, which then fires immediately.
- README: warn that any v<digit> tag, including pre-release tags, deploys
to production; say overrides need an app key; explain how to redeploy
the main app; document the cache_interval range and the 24-hour cache.
The reason will be displayed to describe this comment to others. Learn more.
🟡 Changes recommended
Active moderate issues remain in cache recovery and validation, allowing a full or malformed cache to keep refreshes failing and potentially crash error rendering.
This pull request hardens RSS feed caching by scoping caches to feed URLs, expiring stale entries, handling refresh failures, and clamping refresh intervals. It also updates deployment and configuration documentation.
Addresses Copilot's review of #52.
- loadCache only checked that entries was an array, so a cache like
entries: [null], or an entry without a title, reached render and could
throw instead of showing the error screen. Every entry is now checked:
any malformed entry discards the cache, and missing optional text
fields default to empty.
- Pruning ran after the write, so storage already full of expired caches
made every save fail before pruning could free space. It now runs
first.
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
Active issues remain in src/utils.ts: future-dated cache entries can bypass the intended 24-hour limit, and valid string intervals trigger an erroneous out-of-range warning.
No open issues 🎉
2 previously missed issues
In code that hasn't changed since last review
Future-dated cache entries are incorrectly treated as fresh
src/utils.ts:108
isFresh treats any future timestamp as fresh because a negative age still satisfies <= CACHE_MAX_AGE_MS. A clock correction or a manually preserved cache with a future timestamp can therefore keep stories on screen for longer than the promised 24 hours; require the timestamp to be no later than now before accepting the cache.
String cache intervals trigger a false out-of-range warning
src/utils.ts:196
cache_interval is declared as a string setting (screenly.yml:18-20), and the screenshot configuration also supplies '1800'. The clamp coerces that value to the number 1800, but this strict comparison is against the original string, so every normal startup logs the out-of-range warning even though the interval is valid. Normalize the setting to a number before both clamping and comparing.
A negative age passed the 24-hour check, so a cache saved while the
player's clock was ahead (common before it syncs) stayed fresh for the
whole gap. Only an age from 0 to 24 hours now counts as fresh. Reported
by Copilot on #52.
Following up on Copilot's review of 66ca21a, which listed two previously missed issues:
1. Future-dated caches treated as fresh (src/utils.ts:108): valid, fixed in the next commit.isFresh now accepts only an age from 0 to 24 hours. Players often boot with a wrong date until they sync, so a cache saved while the clock was ahead would otherwise outlive the 24-hour limit. Added a test.
2. String cache_interval triggers a false out-of-range warning (src/utils.ts:196): not an issue, no change. The value reaching getRefreshIntervalMs is already a number. getSettingWithDefault('cache_interval', 1800) has a numeric default, and in @screenly/edge-apps (dist/utils/settings.js) a string setting with a numeric default is parsed with Number(trimmed) and returned as a number. Checked at runtime with the real helper and screenly.settings.cache_interval = '1800':
value=1800 (number) ms=1800000 warnings=0
The browser tests agree: five runs with cache_interval: '1800' logged no warning, and only the run with '0' did.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the cache problems from the independent review of #46, plus the README items from that review.
Cache
The app cached the feed in one shared
localStoragekey (rssStore). It never recorded which feed the cache came from, and never checked its age.rss_urlchanged and the new feed failscache_interval: 0(or 1, or a huge value)setInterval, which then fires immediately)Caches are now stored per feed URL (
rssStore:<url>), each with its URL and save time. When the app saves a cache, it removes the legacy key and other feeds' expired caches, so storage doesn't grow whenrss_urlchanges.One trade-off: the legacy cache doesn't say which feed it belongs to, so it's never used. If a screen's feed is down on its first load after this ships, it shows the error screen instead of the old cache, until the first successful fetch.
A correction to the review: a non-numeric
cache_intervalwas not a loop, becausegetSettingWithDefaultalready falls back to the default for those. The loop came from0, small values andInfinity.README
v<digit>tag, including pre-release tags likev26.11.0-rc.1, deploys to production. This comes from the shared edge-apps-actions tag filter, which every app uses.cache_interval: the 60–86400 range and the 24-hour cache are documented in the README table and in the setting's help text inscreenly.yml.Testing
localStorage), each scenario in the table behaves as listed, plus the legacy-only case (error screen). Withcache_interval: 0, the app fetched once in 3 s.